-
Notifications
You must be signed in to change notification settings - Fork 10
VIDSOL-143: Implement configuration file, VIDSOL-195: Resolve camera LED bug turning on when not publishing video #205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d592f0b to
643099f
Compare
|
SonarCloud failures look to be unrelated to this PR - code duplication on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a configurable features system for the VERA application using a config.json file. The configuration allows administrators to enable/disable various UI elements and behaviors without code changes, including camera/microphone controls, background effects, device selection, layout modes, and meeting room buttons.
Key changes:
- Adds a new
ConfigProvidercontext to manage application configuration - Implements config loading from
frontend/public/config.jsonwith fallback defaults - Updates components to conditionally render based on configuration settings
Reviewed Changes
Copilot reviewed 54 out of 56 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
config.example.json |
Example configuration file showing all available settings |
frontend/src/Context/ConfigProvider/ |
New configuration context and hooks for managing app settings |
frontend/src/hooks/useConfigContext.ts |
Hook to access configuration context |
frontend/src/types/session.ts |
Moved LayoutMode type to shared location |
| Various component files | Updated to conditionally render based on configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...nd/src/Context/BackgroundPublisherProvider/useBackgroundPublisher/useBackgroundPublisher.tsx
Outdated
Show resolved
Hide resolved
| <Tooltip title={tooltipTitle} aria-label={t('devices.settings.ariaLabel')}> | ||
| <div> | ||
| <IconButton | ||
| onClick={handleDeviceStateChange} | ||
| disabled={isButtonDisabled} | ||
| edge="start" | ||
| aria-label={isAudio ? 'microphone' : 'camera'} | ||
| size="small" | ||
| className="m-[3px] size-[50px] rounded-full shadow-md" | ||
| > | ||
| {renderControlIcon()} | ||
| </IconButton> | ||
| </div> | ||
| </Tooltip> |
Copilot
AI
Sep 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The extra <div> wrapper around the IconButton appears to be added only to make the tooltip work with disabled buttons. Consider using a more semantic approach or adding a comment explaining why this wrapper is necessary.
| } | ||
| disconnect(); | ||
| }, [disconnect]); | ||
| destroyBackgroundPublisher(); |
Copilot
AI
Sep 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The destroyBackgroundPublisher call in the disconnect handler should include error handling, as destroying publishers can potentially throw exceptions that would prevent the disconnect from completing.
| destroyBackgroundPublisher(); | |
| try { | |
| destroyBackgroundPublisher(); | |
| } catch (error) { | |
| console.error('Failed to destroy background publisher:', error); | |
| } |
behei-vonage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impressive job 👏 mostly left some nits and a few suggestions. let me know what you think! 🙏
...nd/src/Context/BackgroundPublisherProvider/useBackgroundPublisher/useBackgroundPublisher.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/MeetingRoom/ReduceNoiseTestSpeakers/ReduceNoiseTestSpeakers.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/ScreenSharingButton/ScreenSharingButton.tsx
Outdated
Show resolved
Hide resolved
...components/WaitingRoom/BackgroundEffects/BackgroundEffectsButton/BackgroundEffectsButton.tsx
Outdated
Show resolved
Hide resolved
czoli1976
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some considerations about the nameing of the variables, defenitely a NIT
| @@ -0,0 +1,25 @@ | |||
| { | |||
| "videoSettings": { | |||
| "allowBackgroundEffects": true, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest renaming it to "allowVideoEffects" because, while currently these effects apply only to the background, the same panel could potentially be used in the future for other effects (e.g., lighting correction, eye correction).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this as a team during design review and considered Visual Effects instead of Background Effects before deciding on this naming scheme. All of our components reference BackgroundEffects and everything is currently a background effect. Let's keep as-is for now, seems like premature optimization to rename this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok @cpettet !
OscarFava
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I have just a few minor comments.
frontend/src/components/MeetingRoom/DeviceControlButton/DeviceControlButton.tsx
Show resolved
Hide resolved
OscarFava
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! I leave a minor suggestion!
behei-vonage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! ![]()
|
OscarFava
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
behei-vonage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
I tested the PR and overall it looks good, but I noticed a couple of things that seem a bit off:
Not sure if this is something we actually need to fix, but it stood out to me. We might also want to add comments in the config.example.json file to clarify:
This would make it easier for others to understand the available options when configuring defaults. |
Oh, good catch. This would require a lot of rewiring. Could this be in a follow-up PR?
Yeah, this one is a little strange. We have a configurable prop for device selection, |
|
The comments on usage will be in a separate PR. It should be this ticket: VIDSOL-165 |
|
Thanks @cpettet!! merging this PR. |



What is this PR doing?
Implements configurable features in the VERA application. Documentation will be in a follow-up PR as this one is already long in the tooth. Configuration is handled through a
config.jsonfile that can be moved to thefrontend/publicfolder; there's an example in the base directory,config.example.json. Changes to the config file will be reflected immediately in most cases, but some rooms will need to be re-joined to take effect (ie setting the default layout, audio on join, etc.).I dislike combining tickets, but the issue for the camera LED remaining on when the publisher was not publishing video was difficult to decouple from the configuration API, so it's resolved here, too. It's done by moving the BackgroundPublisherProvider up the tree, removing the need to create --> destroy --> recreate --> destroy it when moving from the waiting room to the meeting room. This simplifies the flow so it's easier to understand, too. 🙌
How should this be manually tested?
Testing configuration
cp config.example.json frontend/public/config.json320x180to see the difference 😬)active-speakerorgrid)rm -rf frontend/public/config.jsonTesting bugfix
What are the relevant tickets?
A maintainer will add this ticket number.
Resolves VIDSOL-143 and VIDSOL-195
Checklist
[✅] Branch is based on
develop(notmain).[ ] Resolves a
Known Issue.[ ] If yes, did you remove the item from the
docs/KNOWN_ISSUES.md?[ ] Resolves an item reported in
Issues.If yes, which issue? Issue Number?